Skip to content
This repository was archived by the owner on Mar 13, 2025. It is now read-only.

fix: issue #72 #75

Merged
merged 2 commits into from
Jul 21, 2021
Merged

fix: issue #72 #75

merged 2 commits into from
Jul 21, 2021

Conversation

yoution
Copy link
Contributor

@yoution yoution commented Jul 21, 2021

@maxceem please review

maxceem
maxceem suggested changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoution So far there are 2 issues mentioned in the comments #72 (comment)

  1. Don't include workPeriods.isFirstWeek and workPeriods.isLastWeek into the fields because there are no such fields. We can only filter by these params.

  2. &billingAccountId=0 is included twice inside the request which is converted to array in the API side. &billingAccountId=0 should be included only once in URL.

@yoution
Copy link
Contributor Author

yoution commented Jul 21, 2021

@maxceem https://github.com/topcoder-platform/micro-frontends-taas-admin-app/blob/dev/src/services/workPeriods.js#L104
the buildRequestQuery is a little disgusting， it will add two billingAccountId, and will remove workPeriods.isFirstWeek and workPeriods.isLastWeek if I not put them in fields

@maxceem
Copy link
Contributor

maxceem commented Jul 21, 2021

@yoution feel free to update it as needed to support our new case.

@yoution yoution requested a review from maxceem July 21, 2021 13:34
maxceem
maxceem approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoution works great! Merging to pass it for QA.

@maxceem maxceem merged commit 6c50177 into topcoder-archive:dev Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maxceem maxceem maxceem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yoution @maxceem